Skip to content

Add PEP 484 type hints to transpiler analysis and utility passes#15832

Merged
ShellyGarion merged 36 commits intoQiskit:mainfrom
Hadar01:feat/add-type-hints-analysis-passes
Apr 28, 2026
Merged

Add PEP 484 type hints to transpiler analysis and utility passes#15832
ShellyGarion merged 36 commits intoQiskit:mainfrom
Hadar01:feat/add-type-hints-analysis-passes

Conversation

@Hadar01
Copy link
Copy Markdown
Contributor

@Hadar01 Hadar01 commented Mar 18, 2026

What this PR does

Adds PEP 484 type annotations and improves docstrings across 12 transpiler
analysis and utility passes:

Analysis passes (qiskit/transpiler/passes/analysis/):
CountOps, Depth, Size, Width, NumQubits, NumTensorFactors,
CountOpsLongestPath, DAGLongestPath, ResourceEstimation

Utility passes (qiskit/transpiler/passes/utils/):
DAGFixedPoint, FixedPoint, ContainsInstruction

Specific changes

  • Add from __future__ import annotations to all 12 files
  • Type-annotate __init__ parameters (bool, str, set[str]) and
    run() signatures (dag: DAGCircuit) with -> None return types
  • Expand sparse docstrings with Args blocks where missing
  • Fix incorrect module-level docstring in contains_instruction.py
    (was a copy-paste leftover: "Check if a property reached a fixed point")
  • Rename the placeholder parameter _ to dag in
    ResourceEstimation.run() for clarity and consistency

Motivation

These passes are part of the public API and used across the transpiler
pipeline, yet they lacked type annotations — hurting IDE auto-completion,
static analysis (mypy / pyright), and Sphinx-generated API documentation.
This is a low-risk, documentation-quality improvement with zero behavioral
changes
.

How it was tested

  • ruff check — all checks passed
  • black --check — 13 files left unchanged
  • AST validation script — confirmed from __future__ import annotations
    and typed run() return annotations present in all 12 files
  • Changes are purely additive type hints and docstrings; CI will validate
    the full test suite

AI attribution

The preparation of this contribution was assisted by Claude Sonnet 4.6

Add `from __future__ import annotations` and PEP 484 type annotations to all 9 analysis passes (CountOps, Depth, Size, Width, NumQubits, NumTensorFactors, CountOpsLongestPath, DAGLongestPath, ResourceEstimation) and 3 utility passes (DAGFixedPoint, FixedPoint, ContainsInstruction).

Changes include:

- Type-annotate __init__ parameters and run() method signatures with DAGCircuit, bool, str, and -> None return types

- Expand sparse docstrings with Args blocks where missing

- Fix copy-paste module docstring in contains_instruction.py (was incorrectly 'Check if a property reached a fixed point')

- Rename placeholder parameter '_' to 'dag' in ResourceEstimation.run() for clarity
@Hadar01 Hadar01 requested a review from a team as a code owner March 18, 2026 22:22
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Mar 18, 2026
@qiskit-bot
Copy link
Copy Markdown
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 18, 2026

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@jakelishman
Copy link
Copy Markdown
Member

This is a pre-written response.

I believe this may have been LLM generated without attribution.

Please confirm the model used, and that the PR was tested and checked by a human before submission, including validating that the PR truly solves the issue at it was described, and takes into account any comments from maintainers in the base issues.

@Hadar01
Copy link
Copy Markdown
Contributor Author

Hadar01 commented Mar 19, 2026

@jakelishman
Screenshot 2026-03-19 141502
Thanks for flagging this. I used an AI assistant (Claude) to help generate the type hint changes. I want to be transparent about that.That said, I did review the generated output, understood the changes, and ran the relevant tests locally to verify nothing was broken:
python -m stestr run test.python.transpiler.test_count_ops_pass test.python.transpiler.test_fixed_point_pass test.python.transpiler.test_count_ops_longest_path_pass test.python.transpiler.test_resource_estimation_pass
Result: 12 passed, 0 failed, 0 skipped.
Also This was a proactive improvement, not tied to an open issue.
I believe the PR correctly adds PEP 484 type hints as described and doesn't break any existing functionality. Happy to make adjustments based on your review

@Hadar01
Copy link
Copy Markdown
Contributor Author

Hadar01 commented Mar 21, 2026

Hi @jakelishman ,I've addressed the transparency concern regarding AI assistance, ran all relevant tests locally (12 passed, 0 failed), and believe the changes are correct. Would appreciate your review when you get a chance. Happy to make any adjustments.

Comment on lines 30 to 31
"""Initialize a ``ContainsInstruction`` pass.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our current suggestion is actually to just remove this initial sentence in __init__ if it doesn't provide any additional information

Suggested change
"""Initialize a ``ContainsInstruction`` pass.
"""

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, removed


def __init__(self, instruction_name, recurse: bool = True):
"""ContainsInstruction initializer.
def __init__(self, instruction_name: str | set[str], recurse: bool = True) -> None:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change from Iterable to set? That seems more restrictive

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original docstring already documented this as str | Iterable[str] so I've matched the type hint to that , changed back to str | Iterable[str] using collections.abc.Iterable.

def run(self, dag):
"""Run the CountOps pass on `dag`."""
def run(self, dag: DAGCircuit) -> None:
"""Run the CountOps pass on *dag*."""
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*dag* is incorrect here, we use double quotes for code formatting (as you/Claude correctly did in other places):

``dag``

This applies to a bunch of other places too

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed,updated to dag across all affected files.


"""Count the operations in a DAG circuit."""

from __future__ import annotations
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These imports don't seem necessary in every file, could you only add them when required?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced from __future__ import annotations with a TYPE_CHECKING guard instead ,this way DAGCircuit is only imported during static analysis and has zero runtime cost. Happy to remove it entirely if you'd prefer to keep the diff minimal.

Comment thread qiskit/transpiler/passes/analysis/resource_estimation.py Outdated
* Size()
* CountOps()
* NumTensorFactors()
* NumQubits()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nice catch 👍🏻

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 😊

Copy link
Copy Markdown
Contributor Author

@Hadar01 Hadar01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the thorough review @Cryoris ! I've pushed a follow-up commit addressing all feedback:

1.Replaced from future import annotations with TYPE_CHECKING guard in all files
2.Fixed dagdag across all files
3.Removed redundant init summary lines
4.Changed set[str] → str | Iterable[str] in contains_instruction.py
5.Removed the no-op docstring from resource_estimation.py

Please take another look when you get a chance!

@Hadar01 Hadar01 requested a review from Cryoris March 26, 2026 07:07
Copy link
Copy Markdown
Collaborator

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates, just one thing about line removals and then this LGTM

# Any modifications or derivative works of this code must retain this
# copyright notice, and modified files need to carry a notice indicating
# that they have been altered from the originals.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it's much easier to see the module docstring with this whitespace, could you leave these as they were?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that! Restored the blank lines , will keep whitespace as in the originals going forward.

"""Run the FixedPoint pass on ``dag``."""
current_value = self.property_set[self._property]
fixed_point_previous_property = f"_fixed_point_previous_{self._property}"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it'd be best to just revert all these line removals generally 😄

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that! Restored the blank lines ,will keep whitespace as in the originals going forward.

Copy link
Copy Markdown
Contributor Author

@Hadar01 Hadar01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Cryoris Done!

@Hadar01 Hadar01 requested a review from Cryoris March 26, 2026 09:57
Hadar01 and others added 3 commits March 28, 2026 06:58
Add from __future__ import annotations to all 12 files so that
type hints are treated as strings at runtime (fixing Python 3.10
NameError). Keep TYPE_CHECKING guard for DAGCircuit import to
satisfy ruff F821. This matches the pattern used in basepasses.py
and layout.py.

Also fixes contains_instruction.py module docstring (was copy-paste
from fixed_point.py).
@Hadar01
Copy link
Copy Markdown
Contributor Author

Hadar01 commented Mar 30, 2026

Hey @Cryoris , so I found out why CI was failing , using only the TYPE_CHECKING guard without from future import annotations breaks on Python 3.10 because the type hints get evaluated at runtime and DAGCircuit isn't actually imported. I've added back from future import annotations alongside the guard in all files. Looked at how basepasses.py and layout.py handle it and they use the same combo, so I followed that pattern. Also caught that contains_instruction.py still had the wrong module docstring from fixed_point.py, fixed that too. Let me know if this looks good!

@Cryoris
Copy link
Copy Markdown
Collaborator

Cryoris commented Apr 24, 2026

Thanks for the updates, this looks good now. Could you update the PR description with the Claude model used (if you know it)?

@Hadar01
Copy link
Copy Markdown
Contributor Author

Hadar01 commented Apr 25, 2026

Thanks for the updates, this looks good now. Could you update the PR description with the Claude model used (if you know it)?

Done @Cryoris

@Cryoris Cryoris enabled auto-merge April 27, 2026 12:49
@Cryoris Cryoris disabled auto-merge April 27, 2026 12:50
@coveralls
Copy link
Copy Markdown

coveralls commented Apr 27, 2026

Coverage Report for CI Build 25011313525

Coverage increased (+0.006%) to 87.576%

Details

  • Coverage increased (+0.006%) from the base build.
  • Patch coverage: 43 of 43 lines across 12 files are fully covered (100%).
  • 8 coverage regressions across 2 files.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

8 previously-covered lines in 2 files lost coverage.

File Lines Losing Coverage Coverage
crates/qasm2/src/lex.rs 6 92.03%
crates/circuit/src/parameter/symbol_expr.rs 2 74.17%

Coverage Stats

Coverage Status
Relevant Lines: 119883
Covered Lines: 104989
Line Coverage: 87.58%
Coverage Strength: 982238.82 hits per line

💛 - Coveralls

@Cryoris Cryoris enabled auto-merge April 27, 2026 13:35
@Cryoris Cryoris changed the title docs: add PEP 484 type hints to transpiler analysis and utility passes Add PEP 484 type hints to transpiler analysis and utility passes Apr 27, 2026
@Cryoris Cryoris added the Changelog: None Do not include in the GitHub Release changelog. label Apr 27, 2026
@Cryoris Cryoris added this pull request to the merge queue Apr 27, 2026
@ShellyGarion ShellyGarion added this to the 2.5.0 milestone Apr 27, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Apr 27, 2026
@ShellyGarion ShellyGarion enabled auto-merge April 28, 2026 05:48
@ShellyGarion ShellyGarion added this pull request to the merge queue Apr 28, 2026
Merged via the queue into Qiskit:main with commit b045984 Apr 28, 2026
45 of 47 checks passed
@github-project-automation github-project-automation Bot moved this from Ready to Done in Qiskit 2.5 Apr 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changelog: None Do not include in the GitHub Release changelog. Community PR PRs from contributors that are not 'members' of the Qiskit repo

Projects

Status: Done
Status: Done

Development

Successfully merging this pull request may close these issues.

7 participants